-
Notifications
You must be signed in to change notification settings - Fork 6k
Update EIP-5792: Propose changes to EIP-5792 atomicity #9529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update EIP-5792: Propose changes to EIP-5792 atomicity #9529
Conversation
|
✅ All reviewers have approved. |
EIPS/eip-5792.md
Outdated
| ### `atomicity` Capability | ||
| Like the illustrative examples given above and other capabilities to be defined in future EIPs, the capability to specify how calls delivered via the above-defined methods should be executed can be expressed as `atomic` or `non-atomic`. | ||
| This capability is expressed separately on each chain and should be interpreted as a guarantee only for batches of transactions on that chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this supposed to be how the wallet expresses the values for atomicity it supports? if so think we could reword to make a bit clearer, lmk if you had something else in mind though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that was the idea...
I have kept the details on the how the flow is controlled, because I believe that those are implementation details that can be better expressed via the flow-control capability. I have prioritised simplicity over configuration. Meaning that the Wallet can implement the both the atomic and non-atomic option as it sees fit. As long as it respects the atomicity for the calls execution.
Re-worded it a bit. Let me know what you think.
EIPS/eip-5792.md
Outdated
| id?: string; | ||
| from?: `0x${string}`; | ||
| chainId: `0x${string}`; // Hex chain id | ||
| atomicity: "atomic" | "nonAtomic" | "any"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it would be nice to have atomicPreferred for when it's preferred but not required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it ever not be preferred? 🤔 Both from Wallet and App perspective..
The idea for any, is for when an App does not care about execution, but really just wants to send a batch of transactions to the wallet.
Also I have not added the option for the Wallet to support both non-atomic and atomic execution for the same chain (see here). The reason was exactly that if atomic is available then the calls will be executed atomically.
Do you think that there's a case where:
- A wallet would prefer sequential execution on a chain where it supports atomic execution?
- An App would prefer sequential execution when it can ask for atomic execution?
And if yes, do you feel that it should be included in the spec, or that it is specific enough that it could be its own capability (defined as a different erc)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that I can't think of a situation where atomic wouldn't be preferred - in which case I think maybe atomic | any might be sufficient (or a boolean as @matthewwalsh0 suggests below?)
Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>
Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>
azf20
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for the updates
EIPS/eip-5792.md
Outdated
| id?: string; | ||
| from?: `0x${string}`; | ||
| chainId: `0x${string}`; // Hex chain id | ||
| atomicity: "atomic" | "nonAtomic" | "any"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that I can't think of a situation where atomic wouldn't be preferred - in which case I think maybe atomic | any might be sufficient (or a boolean as @matthewwalsh0 suggests below?)
EIPS/eip-5792.md
Outdated
| * If a wallet executes multiple calls **atomically** in a single transaction, `wallet_getCallsStatus` MUST return an object with a `receipts` field that contains a single transaction receipt, corresponding to the transaction in which the calls were included. It also MUST be explicit and return an `atomicity` field set to `"strict"`. | ||
| * If a wallet executes multiple calls **atomically** in a multiple transactions as part of a bundle, `wallet_getCallsStatus` MUST return an object with a `receipts` field that contains **an array of receipts** for all transactions. It also MUST be explicit and return a `atomicity` field set to `"loose"`. | ||
| * If a wallet executes multiple calls **non-atomically**, `wallet_getCallsStatus` MUST return an object with a `receipts` field that contains **an array of receipts** for all transactions containing batch calls that were included onchain. This includes the batch calls that were included on-chain but eventually reverted. It also MUST be explicit and return a `atomicity` field set to `"none"`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why specify anything about the structure of the receipt field? It seems overly restrictive. What benefits does it bring?
Might be a question for @lukasrosario too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benefit was to try to make the reasoning about the receipts structure simple and comprehensive enough for dapps. Reduce the complexity and any ambiguity that might exist. However I feel that we can make it less restrictive, as I agree with your previous comment about strict being executed as multiple transactions. At that point, then the receipts field can have:
- a single or multiple transactions for
strictatomicity - multiple transactions for
looseandnone
EIPS/eip-5792.md
Outdated
| * When atomicity is set to `none`: | ||
| * MAY execute each call as a single transaction. | ||
| * MAY execute each call as a single transaction, as part of a bundle that is executed atomically. | ||
| * MAY execute all calls sequentially without any atomicity guarantees. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't specify the behaviour on failed transactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that we need to specify the behaviour for failed transactions submitted sequentially?
I was thinking that we could leave that for the wallets to decide. Meaning that in the simplest form, an app that does not care about atomic execution, will need to go through the receipts and figure out the batch was executed. I would leave further specifications to a different capability (like the flow-control one).
Co-authored-by: Francisco Giordano <fg@frang.io>
Co-authored-by: Francisco Giordano <fg@frang.io>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
EIPS/eip-5792.md
Outdated
| ## Security Considerations | ||
| App developers MUST NOT assume that all calls will be sent in a single transaction if they were submitted to a wallet with a capability defining how to execute the calls non-atomically. The exact behavior of non-atomically executed calls is to be defined in separate ERCs. | ||
| Regardless of the `atomic` value specified, App developers MUST NOT assume that all calls will be sent in a single transaction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an example like the one you shared with me would be good to add here -- i think it might not be immediately obvious why this would be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definition above states:
- MUST execute all calls atomically—either all calls execute successfully or no material effects appear on chain.
Can you give an example of an implementation of "atomic:true" that matches that assert, and still is not on a single transaction ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drortirosh see my comment here
Co-authored-by: Jakub Witczak <jakubwitczak6@gmail.com>
EIPS/eip-5792.md
Outdated
| ```json | ||
| { | ||
| "0x2105": { | ||
| "atomic": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry if i missed something, but why is atomic differently-shaped than other caps, i.e. why are these
"atomic": true
instead of
"atomic": { "supported": true }
like the other examples above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other capabilities highlighted in the spec are just examples of what we might have, but they are not real capabilities defined anywhere. Also there is no specified type for the capability itself (currently is specified as any). Only for the getCapabilities response:
type GetCapabilitiesResult = Record<`0x${string}`, <Record<string, any>>; // Hex chain idI opted for a simple boolean just for simplicity.
Do you think we should change the capability type from any to be something more structured?
My personal opinion is that we should keep it unconstrained, because adopting a capability will be an intended action. So app and wallet will need to specifically develop and implement such capability. And if that is the case, then we should, at least for now, not constrain capability authors with a specified schema/structure.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should keep the GetCapabilitiesResult type as is but agree we should conform to "atomic": { "supported": true }. i dont think that format needs to be a strict requirement, but i dont see why we'd stray from it
bumblefudge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see in-line nit about shape of "atomic" capability
lukasrosario
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lfg
eth-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
Goal
Improve the current API to support, in an expressive and simple way, both atomic and non-atomic batch requests.
Context
The atomic topic has been heavily discussed within the latest 5792 calls. And it was agreed that in order to foster broader adoption of the proposed wallet APIs specified in this spec, it should support both
atomicandnon-atomicbatches.Changes
Introduce a top level
atomicproperty across the different API methods (wallet_sendCalls,wallet_getCallsStatus,wallet_getCapabilities).Wallets express the
atomicvalues it supports viawallet_getCapabilities.Thestrict,looseandnoneatomicity concepts are directly taken from the flow-control capability proposed by @SamWilsn.